Expand altrep in assign#5401
Conversation
tlapak
requested review from
HughParsonage,
MichaelChirico and
jangorecki
as code owners
|
|
||
| 1. `unique()` returns a copy the case when `nrows(x) <= 1` instead of a mutable alias, [#5932](https://github.com/Rdatatable/data.table/pull/5932). This is consistent with existing `unique()` behavior when the input has no duplicates but more than one row. Thanks to @brookslogan for the report and @dshemetov for the fix. | ||
|
|
||
| 2. `data.table` did not always expand ALTREP columns when assigning a full column by reference. This could result in the target column getting modified unintentionally if the next call to the data.table was a modification by reference of the source column. E.g. in `DT[, b := as.character(a)]` the string conversion gets deferred and subsequent modification of column `a` would also modify column `b`, [#5400](https://github.com/Rdatatable/data.table/issues/5400). Thanks to @aquasync for the report and Václav Tlapák for the PR. |
There was a problem hiding this comment.
Can we be more specific? "did not always", well, when?
Trying to put myself in the shoes of a user scanning the NEWS for bugs that might affect me, this one might seem "scary" without more clarity.
| (TYPEOF(values)==VECSXP && i>LENGTH(values)-1) || // recycled RHS would have columns pointing to others, #185. | ||
| (TYPEOF(values)!=VECSXP && i>0) // assigning the same values to a second column. Have to ensure a copy #2540 | ||
| (TYPEOF(values)!=VECSXP && i>0) || // assigning the same values to a second column. Have to ensure a copy #2540 | ||
| ALTREP(thisvalue) |
There was a problem hiding this comment.
a comment here on why this test exists would be appropriate IMO
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5401 +/- ##
=======================================
Coverage 98.69% 98.69%
=======================================
Files 79 79
Lines 14678 14679 +1
=======================================
+ Hits 14487 14488 +1
Misses 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No obvious timing issues in HEAD=expand_ALTREP_in_assign Generated via commit 6777191 Download link for the artifact containing the test results: ↓ atime-results.zip
|
MichaelChirico
left a comment
MichaelChirico
left a comment
There was a problem hiding this comment.
Thanks for the fix! I'm not reproducing the testFun() giving ALTREP locally, but it still passes the test...
3 participants
Closes #5400
Note that I modified test 754.5 (now 754.05) because the character conversion (now tested in 754.07 and 754.08) gets deferred which results in the same behaviour as in the report, while
as.numericdoesn't get deferred.Tests 754.05, 754.07 and 754.09 depend on ALTREP behaviour of R and could break if that changes without
data.tablebeing broken, e.g. ifas.numericgets deferred at some point in the future, the test will break but everything will still work correctly.